Fix issue: #963 UI Overflow on Mobile for Home page#1030
Fix issue: #963 UI Overflow on Mobile for Home page#1030arkid15r merged 11 commits intoOWASP:mainfrom
Conversation
Summary by CodeRabbit
WalkthroughThe pull request refactors the Home page component. The changes include updates to the layout and styling, such as adding extra padding and a new background color to the outer container. The MultiSearchBar component now has a reordered prop list with the removal of the eventData prop. Grid layouts have been adjusted for Upcoming Events (now three columns) and for the New Chapters and New Projects sections (now two columns). Moreover, the TopContributors component now displays six contributors instead of nine, and a new section inviting users to join OWASP has been added. Changes
Suggested Reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
frontend/src/pages/Home.tsx (1)
10-19:⚠️ Potential issueRemove unused import
EventType.The
EventTypeimport on line 18 is declared but never used in this file, as flagged by ESLint in the pipeline failure.- import { EventType } from 'types/event'🧰 Tools
🪛 GitHub Actions: Run CI/CD
[error] 10-10: 'EventType' is defined but never used @typescript-eslint/no-unused-vars
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/pages/Home.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Run CI/CD
frontend/src/pages/Home.tsx
[error] 10-10: 'EventType' is defined but never used @typescript-eslint/no-unused-vars
🔇 Additional comments (5)
frontend/src/pages/Home.tsx (5)
111-113: Good container restructuring to fix mobile overflow.The changes to the container structure address the mobile overflow issue effectively. Moving from a fixed-width outer container to a padded container with a max-width inner wrapper is a good responsive design approach.
131-183: Good restructuring of chapter and project cards.The implementation of separate cards for new chapters and projects with appropriate formatting and consistent styling improves the layout organization and readability on mobile devices.
184-184: Appropriate reduction in displayed contributors.Reducing the
maxInitialDisplayfrom 9 to 6 forTopContributorsis a good adjustment that will help prevent overcrowding on mobile screens.
192-221: Good enhancement with conditional comment count display.The added conditional rendering for comment counts in the ItemCardList components is a nice improvement that prevents displaying empty or zero counts.
223-249: Good responsive grid layout for statistics.Using a responsive grid that switches from 1 column on mobile to 4 columns on medium screens ensures the statistics display properly on all device sizes.
KaranNegi20Feb
force-pushed
the
fixing-home-overflow
branch
2 times, most recently
from
88066b1 to
2a34af3
Compare
KaranNegi20Feb
force-pushed
the
fixing-home-overflow
branch
from
78a0449 to
886e785
Compare
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
What issue is this PR related to?
|
|
Hey @arkid15r all the merge conflicts are resolved, you can now proceed with your review. Thank you. |
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
Thanks for updating mobile view! I like that!
But I noticed a few issues that need to be addressed ⬇️
KaranNegi20Feb
force-pushed
the
fixing-home-overflow
branch
from
4f2d772 to
9d5f492
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/src/pages/Home.tsx (1)
113-113: Consider more responsive padding for the main container.The padding of
p-8(32px) might be excessive on very small mobile screens. Using responsive padding would provide better spacing across different device sizes.- <div className="mt-16 min-h-screen p-8 text-gray-600 dark:bg-[#212529] dark:text-gray-300"> + <div className="mt-16 min-h-screen p-4 md:p-8 text-gray-600 dark:bg-[#212529] dark:text-gray-300">
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/e2e/pages/Contribute.spec.ts(1 hunks)frontend/src/pages/Home.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/tests/e2e/pages/Contribute.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (5)
frontend/src/pages/Home.tsx (5)
114-114: LGTM! Great solution for preventing overflow.Adding a centered, max-width container is an excellent approach to address the mobile overflow issue. This ensures content is properly constrained across all device sizes.
160-212: Good implementation of responsive grid.The two-column layout with
md:grid-cols-2properly collapses to a single column on mobile screens, preventing horizontal overflow. The consistent use of this pattern throughout the page is excellent for maintaining responsive behavior.
213-213: Reduced the initial contributors display count.The
maxInitialDisplayprop for TopContributors has been reduced from 9 to 6, which should help with mobile display by showing fewer items initially.
251-260: Good responsive implementation for counter stats.The grid layout with
md:grid-cols-4ensures the counter stats are stacked vertically on mobile screens and displayed in a row on larger screens, which is appropriate for mobile viewing.
277-279: Moving logos implementation preserved as requested.As noted in the past review comments, the MovingLogos component has been correctly preserved and properly integrated into the new layout structure.
aramattamara
left a comment
aramattamara
left a comment
There was a problem hiding this comment.
Tested on local envirnment, looks good! Thanks for this PR!
welcome @aramattamara 😄. |
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
It seems that there are still some changes to the home page that are not rellevant to this task.
-
We used to have 3 columns for
Upcoming events, now it shows two:
-
Recent issues and releases are still showing up in rows instead of having two columns side by side:
|
Hey @kasya I checked the code push, everything seems fine here, the branch runs fine on my device as well, if you could show me what's that you saw that does not match the right version. Thank you. |
KaranNegi20Feb
force-pushed
the
fixing-home-overflow
branch
from
b01c252 to
ab7f5b9
Compare
|
This is how it looks on my local machine. Screen.Recording.2025-03-10.at.10.24.35.AM.mov |
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Please keep truncate class for long titles.
Truncated the long titles: mobile.overflow.1.movHey @arkid15r i have made the required changes you may now go through the PR to review it. Thank you. |
KaranNegi20Feb
force-pushed
the
fixing-home-overflow
branch
from
79f6de7 to
94e45fc
Compare
|
Hey @arkid15r you can now review the changes, also fixed the merge conflicts, Thank you. |
|
|
Hey @arkid15r I have made also covered the merge conflicts, please review the PR. Thank you. |
|
The results of the PR: Screen.Recording.2025-03-12.at.2.23.23.AM.movScreen.Recording.2025-03-12.at.2.23.43.AM.mov |
arkid15r
dismissed
kasya’s stale review
I checked it locally and it looks fine.
* fixing issues merge-conflicts and tests * Fix: home page issue and errors * Fix: home page issue and fixing ci/cd error * changes made to reviews * fixing the reviews 10March * fixing the reviews 10March * added truncate to recent events * new resolved merge conflicts
4 participants
Describe the bug:
The page width and the container width do not match properly, causing horizontal overflow on mobile devices.
Fix results:
Fix_mobile.mov